Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removes custom established VPs #2561

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Removes custom established VPs #2561

wants to merge 17 commits into from

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Feb 8, 2024

Describe your changes

Closes #2554.

Modifies tx_init_account and tx_update account to not write the vp key. Updates of it are not supported for now and when initializing a new established account (also for validators) the protocol automatically assigns it the vp_user.

The host function update_validity_predicate has been removed.

The accounts' vps have been removed from the genesis files.

Indicate on which release or other PRs this topic is based on

v0.31.8

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

&vp_code_sec.tag,
)?;
}
// TODO: commented out for now cause we don't want to support vp changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the code, don't comment it out. This is what Git is for.

if owner == &addr {
has_post && *valid_sig
// TODO: disabled cause we don't want to support vp changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

grarco added a commit that referenced this pull request Feb 9, 2024
@grarco grarco changed the title Removes vp updates Removes custom established VPs Feb 9, 2024
grarco added a commit that referenced this pull request Feb 10, 2024
@grarco grarco marked this pull request as ready for review February 10, 2024 00:40
tzemanovic
tzemanovic previously approved these changes Feb 12, 2024
true
}
}
KeyType::Vp(owner) => owner != &addr,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwgoes a note on this. This will prevent any vp update even if we wanted to allow it later. To do so we'd need to hard fork and modify this line so that vp updates are enabled. We's also need to modify all the accounts ? storage key to point to a different vp hash. At the same time we can't allow vp changes in here (even though we removed them from the tx) cause it will be possible to to change VP via a sequence of governance proposals

@Fraccaman
Copy link
Member

@cwgoes do we really want to remove the possibility to update vps via governance? can't we just remove the possibility for usersd to update their acccou t vp and have just the default one? vps are whitelisted anyway and if we just remove the possibility from CLI and wasm transaction to update the validity predicate we can retain the possibility to create multiple of them via governance.

@cwgoes
Copy link
Contributor

cwgoes commented Feb 13, 2024

@cwgoes do we really want to remove the possibility to update vps via governance? can't we just remove the possibility for usersd to update their acccou t vp and have just the default one? vps are whitelisted anyway and if we just remove the possibility from CLI and wasm transaction to update the validity predicate we can retain the possibility to create multiple of them via governance.

Yeah - I think we can retain the possibility to update VPs via governance, I just want to remove the user-submitted txs.

grarco added a commit that referenced this pull request Feb 13, 2024
grarco added a commit that referenced this pull request Feb 13, 2024
@grarco grarco force-pushed the grarco/remove-update-vp branch 2 times, most recently from e18ae94 to 6a4d5d1 Compare February 13, 2024 16:51
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 54.00%. Comparing base (2535c9c) to head (3c54841).

Files Patch % Lines
crates/sdk/src/signing.rs 0.00% 4 Missing ⚠️
crates/apps/src/lib/cli.rs 0.00% 2 Missing ⚠️
crates/sdk/src/tx.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2561      +/-   ##
==========================================
+ Coverage   53.95%   54.00%   +0.05%     
==========================================
  Files         308      308              
  Lines      100018    99759     -259     
==========================================
- Hits        53967    53879      -88     
+ Misses      46051    45880     -171     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tzemanovic
tzemanovic previously approved these changes Feb 23, 2024
@grarco
Copy link
Contributor Author

grarco commented Mar 5, 2024

After #2770 we probably need to rethink this a bit since we might need to let user update their vps

@Fraccaman Fraccaman mentioned this pull request Apr 15, 2024
@grarco
Copy link
Contributor Author

grarco commented Apr 23, 2024

About my previous comment: given the way we chose to remove vps from the allowlist, I believe we need to revert the tx_update_account tx to allow the update of the vp in case it was removed from the allowlist. We can still prevent custom vps in genesis instead

@brentstone
Copy link
Collaborator

is this something we still want to include in the near future?

This was referenced May 24, 2024
@brentstone brentstone mentioned this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the ability to update an account's VP
6 participants